Fix calendar, fix page search, fix grouped reactions PFPs#2183
Fix calendar, fix page search, fix grouped reactions PFPs#2183
Conversation
Signed-off-by: prxt6529 <prxt@6529.io>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds UTC-aware month formatting and tests; refactors notification identity deduplication and adds a single-reactor render path; replaces page-search priority with canonical token matching; consolidates TitleContext route/title effects and default for /meme-calendar; and adds/updates related tests. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as rgba(63,81,181,0.5) NotificationDropReactedGroup
participant Logic as rgba(33,150,243,0.5) DeduplicationLogic
participant Avatars as rgba(76,175,80,0.5) OverlappingAvatars
participant Header as rgba(255,152,0,0.5) NotificationHeader
participant FollowAll as rgba(156,39,176,0.5) NotificationsFollowAllBtn
participant FollowOne as rgba(233,30,99,0.5) NotificationsFollowBtn
participant Timestamp as rgba(121,85,72,0.5) NotificationTimestamp
UI->>Logic: provide related_notifications list
Logic-->>UI: grouped latestPerUser + merged identities
alt multiple visible reactors
UI->>Avatars: render avatar items (keys from getIdentityKey)
UI->>FollowAll: render follow-all control
UI->>Timestamp: render timestamp
else single visible reactor
UI->>Header: render NotificationHeader with single reactor author
UI->>FollowOne: render single follow button
UI->>Timestamp: render timestamp and "reacted" label
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: prxt6529 <prxt@6529.io>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx (1)
42-44: Consider using consistent nullish operators for clarity.
handleuses??whilepfpandprimary_addressuse||. This means an empty stringhandlewould be kept, but an empty stringpfporprimary_addresswould fall back. If this is intentional, a brief comment would help future readers understand the distinction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx` around lines 42 - 44, The three fields in NotificationDropReactedGroup mapping use mixed fallback operators—handle uses nullish coalescing (preferred.handle ?? fallback.handle) while pfp and primary_address use logical OR (preferred.pfp || fallback.pfp, preferred.primary_address || fallback.primary_address)—so decide whether empty strings should be treated as valid values; either switch pfp and primary_address to nullish coalescing to match handle (preferred.pfp ?? fallback.pfp, preferred.primary_address ?? fallback.primary_address) or keep the current operators but add a concise comment above these lines explaining why handle preserves empty strings while pfp/primary_address fall back on falsy values.__tests__/components/brain/notifications/drop-reacted/NotificationDropReactedGroup.test.tsx (1)
102-124: Consider creating properly typed mock data instead ofas never.The
as neverassertion hides missing fields fromApiProfileMin(e.g.,banner1_color,cic,rep,tdh,level). While the component may not access these fields, using proper types would catch issues if the component evolves.♻️ Example: Create a minimal mock factory
function createMockProfile(overrides: Partial<ApiProfileMin> & { handle: string }): ApiProfileMin { return { id: `${overrides.handle}-id`, handle: overrides.handle, pfp: null, banner1_color: null, banner2_color: null, cic: 0, rep: 0, tdh: 0, tdh_rate: 0, xtdh: 0, xtdh_rate: 0, level: 0, primary_address: `0x${overrides.handle}`, subscribed_actions: [], ...overrides, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/brain/notifications/drop-reacted/NotificationDropReactedGroup.test.tsx` around lines 102 - 124, The test currently returns a mock profile object cast with "as never", which hides missing fields from the ApiProfileMin type; replace this with a minimal, correctly typed mock factory (e.g., createMockProfile) that returns ApiProfileMin and accepts overrides (Partial<ApiProfileMin> & { handle: string }), populate required fields like id, handle, pfp, banner1_color, banner2_color, cic, rep, tdh, tdh_rate, xtdh, xtdh_rate, level, primary_address, subscribed_actions, and spread overrides; then use createMockProfile(...) in place of the object and remove the "as never" cast so TypeScript will surface any missing properties in functions/components that consume the mock (refer to the returned object in this test and to ApiProfileMin).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/components/meme-calendar/meme-calendar.helpers.timezone.test.ts`:
- Around line 34-36: The afterEach cleanup is restoring process.env.TZ by
assigning originalTz directly, which writes the string "undefined" when
originalTz was unset; modify the afterEach in
meme-calendar.helpers.timezone.test.ts to check originalTz: if it is undefined
delete process.env.TZ, otherwise set process.env.TZ = originalTz, using the
existing afterEach and originalTz identifiers to locate the change.
In `@components/header/header-search/HeaderSearchModal.tsx`:
- Around line 161-172: The plural-singular logic in HeaderSearchModal's token
normalization over-stems by treating any token ending with "ses" as removable;
update the condition in the function that checks token suffixes (the branch that
currently tests token.endsWith("ses")) to a narrower match such as
token.endsWith("sses") so words like "messages" or "cases" are not truncated
incorrectly, leaving the general token.endsWith("s") && !token.endsWith("ss")
fallback to remove simple trailing "s" cases.
In `@components/meme-calendar/meme-calendar.helpers.tsx`:
- Around line 616-623: The month formatting in formatUtcMonth currently calls
d.toLocaleString(undefined, ...) which uses the runtime default locale and can
cause SSR/CSR hydration mismatches; update the function so the toLocaleString
call uses an explicit locale (e.g., 'en-US') or add an optional locale parameter
defaulting to a fixed locale, ensuring the call to d.toLocaleString(...) passes
that explicit locale along with month/style and timeZone: 'UTC' (change inside
function formatUtcMonth to remove undefined and use the explicit locale or
param).
In `@contexts/TitleContext.tsx`:
- Around line 123-126: The current branch only resets the title and waveData
when a wave id disappears; change it to also reset when the wave id changes by
comparing the previous and current wave ids (not just their truthiness). In the
block that uses previousWaveInUrl and currentWaveInUrl, add a condition that
checks previousWaveId !== currentWaveId (derived from the query param used to
build previousWaveInUrl/currentWaveInUrl) and, when they differ, call
setTitle(getDefaultTitleForRoute(pathname)) and setWaveData(null) (same actions
as the disappearance case) so the old wave title is cleared whenever the active
wave id swaps. Ensure you reference the existing variables previousWaveInUrl,
currentWaveInUrl, previousWaveId/currentWaveId, setTitle, setWaveData,
getDefaultTitleForRoute, and pathname when applying the change.
---
Nitpick comments:
In
`@__tests__/components/brain/notifications/drop-reacted/NotificationDropReactedGroup.test.tsx`:
- Around line 102-124: The test currently returns a mock profile object cast
with "as never", which hides missing fields from the ApiProfileMin type; replace
this with a minimal, correctly typed mock factory (e.g., createMockProfile) that
returns ApiProfileMin and accepts overrides (Partial<ApiProfileMin> & { handle:
string }), populate required fields like id, handle, pfp, banner1_color,
banner2_color, cic, rep, tdh, tdh_rate, xtdh, xtdh_rate, level, primary_address,
subscribed_actions, and spread overrides; then use createMockProfile(...) in
place of the object and remove the "as never" cast so TypeScript will surface
any missing properties in functions/components that consume the mock (refer to
the returned object in this test and to ApiProfileMin).
In
`@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx`:
- Around line 42-44: The three fields in NotificationDropReactedGroup mapping
use mixed fallback operators—handle uses nullish coalescing (preferred.handle ??
fallback.handle) while pfp and primary_address use logical OR (preferred.pfp ||
fallback.pfp, preferred.primary_address || fallback.primary_address)—so decide
whether empty strings should be treated as valid values; either switch pfp and
primary_address to nullish coalescing to match handle (preferred.pfp ??
fallback.pfp, preferred.primary_address ?? fallback.primary_address) or keep the
current operators but add a concise comment above these lines explaining why
handle preserves empty strings while pfp/primary_address fall back on falsy
values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07764220-1ebc-42a8-9e22-e5ed4a2a8657
📒 Files selected for processing (9)
__tests__/components/brain/notifications/drop-reacted/NotificationDropReactedGroup.test.tsx__tests__/components/header/HeaderSearchModal.test.tsx__tests__/components/meme-calendar/meme-calendar.helpers.timezone.test.ts__tests__/contexts/TitleContext.test.tsxcomponents/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsxcomponents/header/header-search/HeaderSearchModal.tsxcomponents/meme-calendar/MemeCalendar.tsxcomponents/meme-calendar/meme-calendar.helpers.tsxcontexts/TitleContext.tsx
Signed-off-by: prxt6529 <prxt@6529.io>
Signed-off-by: prxt6529 <prxt@6529.io>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
contexts/TitleContext.tsx (1)
108-109: Optional cleanup: remove redundant wave-id aliases.
currentWaveId/previousWaveIdare direct aliases and can be inlined in the condition to reduce cognitive overhead.Suggested simplification
- const currentWaveId = currentWaveInUrl; - const previousWaveId = previousWaveInUrl; ... - if ( - previousWaveInUrl && - (!currentWaveInUrl || previousWaveId !== currentWaveId) - ) { + if ( + previousWaveInUrl && + (!currentWaveInUrl || previousWaveInUrl !== currentWaveInUrl) + ) {Also applies to: 125-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contexts/TitleContext.tsx` around lines 108 - 109, Remove the redundant aliases currentWaveId and previousWaveId in TitleContext.tsx and inline currentWaveInUrl and previousWaveInUrl directly where those aliases are used (e.g., in the conditional checks around the currentWave/previousWave logic and the block referenced near lines 125-128). Update any expressions or conditions that reference currentWaveId/previousWaveId to use currentWaveInUrl/previousWaveInUrl instead and delete the now-unused const declarations to reduce cognitive overhead and unused variables.components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx (2)
26-33: Empty string edge case may cause unexpected grouping.Per the
ApiProfileMintype,idis a requiredstring. Since nullish coalescing (??) only catchesnull/undefinedand not empty strings, ifidis"", it will be returned as the key. Multiple profiles with emptyidwould then share the same key and be incorrectly merged together.If this is an intentional defensive measure against malformed data, consider using
||instead of??to treat empty strings as "missing":Optional: Use falsy check for defensive empty-string handling
function getIdentityKey(profile: ApiProfileMin): string { return ( - profile.id ?? - profile.handle ?? - profile.primary_address ?? + profile.id || + profile.handle || + profile.primary_address || `unknown-profile` ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx` around lines 26 - 33, getIdentityKey currently uses nullish coalescing (profile.id ?? profile.handle ?? profile.primary_address ?? `unknown-profile`) which will return empty strings as valid keys and can cause incorrect grouping for ApiProfileMin entries with id === ""; change the logic in getIdentityKey to treat empty strings as missing (e.g., use falsy checks or || instead of ??, or explicitly check profile.id.trim() === "" before falling back) so that handle/primary_address/`unknown-profile` are used when id is an empty string.
193-197: Redundant null/undefined check forprofile.id.Per
ApiProfileMin,idis a requiredstring(not nullable). The null/undefined check andString()conversion are unnecessary:Simplify displayName logic
const displayName = - profile.handle ?? - (profile.id === null || profile.id === undefined - ? undefined - : String(profile.id)); + profile.handle ?? profile.id;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx` around lines 193 - 197, The displayName assignment includes an unnecessary null/undefined check and String() conversion for profile.id even though ApiProfileMin defines id as a required string; replace the ternary block with a simple fallback: use profile.handle ?? profile.id (remove the (profile.id === null || profile.id === undefined ? undefined : String(profile.id)) logic) so displayName is either profile.handle or profile.id directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx`:
- Around line 60-62: The loop currently skips notifications when
getIdentityKey(n.related_identity) returns an empty string which silently drops
valid reactions; change the behavior in NotificationDropReactedGroup by
capturing the result of getIdentityKey for each notification (the const key =
getIdentityKey(n.related_identity) inside the for (const n of notifications)
loop) and if it is empty, use a deterministic fallback key (for example
"unknown-identity" or fallback to n.related_identity.profile?.handle ||
n.related_identity.id) and emit a console.warn or use the component logger to
record the missing key before grouping; ensure the rest of the grouping logic
uses this fallback rather than continuing the loop so notifications are not
lost.
In `@components/header/header-search/HeaderSearchModal.tsx`:
- Around line 333-351: The current matcher only tests each candidate field
independently (using hasCanonicalPageTokenMatch and
hasCanonicalPageTokenPrefixMatch over normalizedTitle, normalizedHref,
normalizedBreadcrumbs, normalizedSearchTerms), so queries spanning fields (e.g.,
"metrics health") miss matches; fix this by building and passing extra composite
searchable strings (e.g., join normalizedBreadcrumbs + normalizedTitle into
"network metrics health" and also shorter combinations like "metrics health")
into the same array you pass to the matcher and ensure the identical composite
list is produced and reused by getPageMatchPriority() so matching and ranking
remain aligned (update the construction logic where
normalizedTitle/normalizedBreadcrumbs/normalizedSearchTerms are assembled and
reference that composite-list in both the matcher call and
getPageMatchPriority).
- Around line 272-309: Add an explicit exact-href check so exact full-path
queries win before breadcrumb/token fallbacks: inside the checks array in
getPageMatchPriority (where normalizedTitle, hrefValues, hrefSegments,
normalizedQuery etc. are in scope), insert normalizedHref === normalizedQuery as
a high-priority entry (for example immediately after normalizedTitle ===
normalizedQuery) so an exact match on normalizedHref is found before any
breadcrumb/token-based checks or substring fallbacks.
---
Nitpick comments:
In
`@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx`:
- Around line 26-33: getIdentityKey currently uses nullish coalescing
(profile.id ?? profile.handle ?? profile.primary_address ?? `unknown-profile`)
which will return empty strings as valid keys and can cause incorrect grouping
for ApiProfileMin entries with id === ""; change the logic in getIdentityKey to
treat empty strings as missing (e.g., use falsy checks or || instead of ??, or
explicitly check profile.id.trim() === "" before falling back) so that
handle/primary_address/`unknown-profile` are used when id is an empty string.
- Around line 193-197: The displayName assignment includes an unnecessary
null/undefined check and String() conversion for profile.id even though
ApiProfileMin defines id as a required string; replace the ternary block with a
simple fallback: use profile.handle ?? profile.id (remove the (profile.id ===
null || profile.id === undefined ? undefined : String(profile.id)) logic) so
displayName is either profile.handle or profile.id directly.
In `@contexts/TitleContext.tsx`:
- Around line 108-109: Remove the redundant aliases currentWaveId and
previousWaveId in TitleContext.tsx and inline currentWaveInUrl and
previousWaveInUrl directly where those aliases are used (e.g., in the
conditional checks around the currentWave/previousWave logic and the block
referenced near lines 125-128). Update any expressions or conditions that
reference currentWaveId/previousWaveId to use currentWaveInUrl/previousWaveInUrl
instead and delete the now-unused const declarations to reduce cognitive
overhead and unused variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81208a81-e4f4-45fe-94ff-9f449e4f321b
📒 Files selected for processing (7)
__tests__/components/brain/notifications/drop-reacted/NotificationDropReactedGroup.test.tsx__tests__/components/meme-calendar/meme-calendar.helpers.timezone.test.ts__tests__/contexts/TitleContext.test.tsxcomponents/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsxcomponents/header/header-search/HeaderSearchModal.tsxcomponents/meme-calendar/meme-calendar.helpers.tsxcontexts/TitleContext.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- components/meme-calendar/meme-calendar.helpers.tsx
- tests/contexts/TitleContext.test.tsx
- tests/components/brain/notifications/drop-reacted/NotificationDropReactedGroup.test.tsx
- tests/components/meme-calendar/meme-calendar.helpers.timezone.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx (1)
46-59: Inconsistent empty-string handling forhandlemay cause malformed links and API calls.Using
??forhandle(line 53) preserves empty strings"", while||forpfp/primary_addresstreats them as missing. Ifpreferred.handleis""andfallback.handleis valid, the merged profile retains the empty string.Downstream,
NotificationHeadercreateshref="/"with empty link text, andNotificationsFollowBtncallsidentities//subscriptions—both malformed.Consider using
||consistently, or usegetNonEmptyIdentityValue:Proposed fix
return { ...fallback, ...preferred, - handle: preferred.handle ?? fallback.handle, - // Keep an explicit empty handle, but treat blank avatar/address strings as - // missing so older identity data survives grouped merges. + // Treat blank strings as missing so older identity data survives grouped merges. + handle: getNonEmptyIdentityValue(preferred.handle) ?? fallback.handle, pfp: preferred.pfp || fallback.pfp, primary_address: preferred.primary_address || fallback.primary_address, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx` around lines 46 - 59, mergeProfiles currently uses the nullish coalescing operator for handle (preferred.handle ?? fallback.handle) which preserves empty strings and leads to malformed links and API calls downstream (e.g., NotificationHeader and NotificationsFollowBtn); change the logic in mergeProfiles to treat empty strings as missing by using a truthy check (preferred.handle || fallback.handle) or a helper like getNonEmptyIdentityValue for handle so empty "" falls back to fallback.handle, keeping pfp and primary_address behavior consistent with handle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx`:
- Around line 188-203: The single-reactor rendering path must validate
singleReactor.handle the same way the multi-reactor path does: before passing
singleReactor to NotificationHeader and NotificationsFollowBtn, check
singleReactor.handle and use profile.handle ? `/${profile.handle}` : undefined
(or fall back to rendering the multi-reactor layout) so you don't pass "/null"
or malformed hrefs like "identities//subscriptions"; update the
NotificationDropReactedGroup single-reactor branch to perform this guard and use
the validated/normalized handle value when constructing NotificationHeader and
NotificationsFollowBtn props.
In `@components/header/header-search/HeaderSearchModal.tsx`:
- Around line 301-338: pageMatchesQuery currently performs href substring checks
too late, so path-like partial queries (e.g., "/network/he") can lose to
token-based title matches; update the checks array inside pageMatchesQuery to
move full-href prefix and substring tests (e.g.,
normalizedHref.startsWith(normalizedQuery) and
normalizedHref.includes(normalizedQuery) or similar checks against
hrefValues/hrefSegments) before any token-based fallbacks (functions like
hasCanonicalPageTokenMatch, hasCanonicalPageTokenPrefixMatch,
hasExactCanonicalPageTokenMatch), and only run these full-href checks for
path-like queries (detect via normalizedQuery containing "/" or starting with
"/") so we prioritize direct path matches before token-based ranking.
---
Nitpick comments:
In
`@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx`:
- Around line 46-59: mergeProfiles currently uses the nullish coalescing
operator for handle (preferred.handle ?? fallback.handle) which preserves empty
strings and leads to malformed links and API calls downstream (e.g.,
NotificationHeader and NotificationsFollowBtn); change the logic in
mergeProfiles to treat empty strings as missing by using a truthy check
(preferred.handle || fallback.handle) or a helper like getNonEmptyIdentityValue
for handle so empty "" falls back to fallback.handle, keeping pfp and
primary_address behavior consistent with handle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fbabda0-c9df-48c1-bb8f-e60cf35826a4
📒 Files selected for processing (5)
__tests__/components/brain/notifications/drop-reacted/NotificationDropReactedGroup.test.tsx__tests__/components/header/HeaderSearchModal.test.tsxcomponents/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsxcomponents/header/header-search/HeaderSearchModal.tsxcontexts/TitleContext.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/components/brain/notifications/drop-reacted/NotificationDropReactedGroup.test.tsx
- contexts/TitleContext.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx`:
- Around line 74-78: getIdentityKey/identityKey currently returns a single
preferred key which can split the same person across buckets; modify the
grouping logic in NotificationDropReactedGroup (where identityKey and key are
computed) to first attempt to match an existing byUser entry by any known alias
(n.related_identity.id, n.related_identity.handle,
n.related_identity.primary_address) before creating a new key; if any alias
matches an existing bucket use that bucket's key, otherwise fall back to the
existing identityKey/`unknown-identity-${n.id}` behavior so identical reactors
are grouped even when different fields are populated.
- Around line 96-106: The merge currently discards previously accumulated
profile data when folding an older notification: in the block that computes
isNewer, latest, fallbackIdentity and calls byUser.set, ensure you preserve
existing.identity when n is older than existing.latest by using
existing.identity (not raw existing.related_identity) as one of the inputs to
mergeProfiles; i.e., when isNewer is false set identity =
mergeProfiles(existing.identity, n.related_identity) (and when true keep
mergeProfiles(latest.related_identity, existing.identity)) so the accumulated
handle/pfp from existing.identity is retained before calling byUser.set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcfa2c0a-87b6-4e25-bd53-01ffaf4a3b0d
📒 Files selected for processing (4)
__tests__/components/brain/notifications/drop-reacted/NotificationDropReactedGroup.test.tsx__tests__/components/header/HeaderSearchModal.test.tsxcomponents/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsxcomponents/header/header-search/HeaderSearchModal.tsx
✅ Files skipped from review due to trivial changes (1)
- tests/components/brain/notifications/drop-reacted/NotificationDropReactedGroup.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx (1)
88-94:⚠️ Potential issue | 🟠 MajorFold all matched alias buckets into one
byUserentry.This still misses the case where two partial buckets already exist before a later notification connects them. Here you pick only the first alias hit, update that bucket, and repoint
aliasToKey, but the otherbyUserentry is never merged away, so the same reactor can still render twice.Possible direction
- const matchingKey = getIdentityAliases(n.related_identity).find((alias) => - aliasToKey.has(alias) - ); - const key = - matchingKey === undefined - ? fallbackKey - : (aliasToKey.get(matchingKey) ?? fallbackKey); + const matchedKeys = Array.from( + new Set( + getIdentityAliases(n.related_identity) + .map((alias) => aliasToKey.get(alias)) + .filter((mappedKey): mappedKey is string => mappedKey !== undefined) + ) + ); + const key = matchedKeys[0] ?? fallbackKey; + // Merge/delete any additional matched buckets before applying `n`, + // so `byUser` only keeps one entry per logical reactor.Also applies to: 104-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx` around lines 88 - 94, When assigning a bucket for a reactor you currently only pick the first matching alias (matchingKey) and repoint aliasToKey, which leaves other pre-existing byUser buckets unmerged and causes duplicate renders; instead, collect all aliases from getIdentityAliases(n.related_identity) that exist in aliasToKey, determine one canonical key (e.g., fallbackKey or the first existing key), then merge every other bucket's contents from byUser into that canonical byUser entry, update aliasToKey for each merged alias to point to the canonical key, and delete the now-empty/duplicate byUser entries so all aliases fold into a single bucket.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx`:
- Around line 258-270: Normalize profile.handle once using
getNonEmptyIdentityValue and use that normalized value when building href,
displayName, title, and fallback in NotificationDropReactedGroup (instead of
using profile.handle directly); compute const normalizedHandle =
getNonEmptyIdentityValue(profile.handle) (or similar) and then set href =
normalizedHandle ? `/${normalizedHandle}` : undefined, displayName =
normalizedHandle ?? profile.id, title = displayName || undefined, and fallback =
normalizedHandle?.slice(0,2).toUpperCase() ?? "?" so empty or whitespace-only
handles no longer produce invalid hrefs or empty avatar/tooltips.
---
Duplicate comments:
In
`@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx`:
- Around line 88-94: When assigning a bucket for a reactor you currently only
pick the first matching alias (matchingKey) and repoint aliasToKey, which leaves
other pre-existing byUser buckets unmerged and causes duplicate renders;
instead, collect all aliases from getIdentityAliases(n.related_identity) that
exist in aliasToKey, determine one canonical key (e.g., fallbackKey or the first
existing key), then merge every other bucket's contents from byUser into that
canonical byUser entry, update aliasToKey for each merged alias to point to the
canonical key, and delete the now-empty/duplicate byUser entries so all aliases
fold into a single bucket.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23df69c7-8759-4abf-8828-b7301900a10c
📒 Files selected for processing (2)
__tests__/components/brain/notifications/drop-reacted/NotificationDropReactedGroup.test.tsxcomponents/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/components/brain/notifications/drop-reacted/NotificationDropReactedGroup.test.tsx
|



Summary by CodeRabbit
New Features
Bug Fixes
Tests